-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH] Use TSV for DWI gradients, and allow both voxel- and world- coordinates conventions #352
base: master
Are you sure you want to change the base?
Conversation
I added a table describing BigDelta and SmallDelta. These address @arokem's suggestion #349 (comment). Although I'm not sure if the thread is suggesting a json sidecar for the gradient tsv or adding some additional fields to the dwi's json sidecar. This adds them to the existing sidecar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a good step towards addressing #349. There are two main missing items:
- Implementation of tensor b-encoding; proposed by @jhlegarreta [ENH] Optional alternative to bval/bvec #349 (comment)
- The general discussion about backward compatibility introduced by @chrisgorgo - [ENH] Optional alternative to bval/bvec #349 (comment)
While 1) probably boils down to writing one example, I think 2) deserves careful discussion.
Let me bring here Chris' suggestions:
Require the metadata in either new or old format.
- pros: old datasets are still compatible
- cons: old BIDS apps need to be updated to support the new format, new BIDS apps need to support two formats.
Make new format optional (old format remains compulsory):
- pros: old datasets are still compatible
- cons: BIDS apps will have to keep supporting the old format
Make old format optional (new format becomes compulsory):
- pros: new BIDS apps can take advantage of the new format without having to support the old one
- cons: old BIDS apps need to be updated, old datasets need to be updated
Make old format unsupported (new format becomes compulsory):
- pros: new BIDS apps can take advantage of the new format without having to support the old one
- cons: old BIDS apps need to be updated, old datasets need to be updated
Option 3 could lead to 4 after some depreciation period. Tooling could be built to assist in updating datasets.
Chris framed this in the general BIDS scope, mentioning that the question of how to introduce the new format can be separate from the question of what is the ideal new format. Admins might want to split the discussions for clarity.; however, I think this is a special case.
I am pretty confident we should go with Option 3 followed by 4 (Chris' final comment). In this case, the problem is not just a new format replacing the standard format currently in BIDS. The problem is that the old format (bvec/bval) violates some principles of BIDS and has been shown to be really unreliable (the DWI community is always wary of whether their bvecs are x-flipped, for instance). So I was really pleased by @mattcieslak proposal when I first read it. The tsv format is not just more consistent with BIDS, it is also more reliable to define unequivocally the b-matrix.
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
There was also some discussion about existing software (and particularly BIDS-Apps) that could potentially require changes if this PR were accepted. Some people who might want to have a say here: @bids-standard/derivatives-mri-dwi, @ecaruyer, @jelleveraart. |
Since the discussion has moved here, I asked friends who are using gradient tensor imaging how they store b-tensors and they pointed me to this repository: https://github.com/markus-nilsson/fwf_header_tools. I haven't gone through it in detail yet. |
Thanks for summarizing what has been discussed in the pas few days @oesteban .
Following the link posted by @mattcieslak, I found @filip-szczepankiewicz 's repository's resources section, which seems indeed extremely useful to have freely available data examples, examples of the sequences and tools to deal with them. Have not investigated the implementation (I am not an expert on FWF/not a user yet), what the gradient data looks like, or how these fit into the
I also see 3 followed by 4 as the way to go. |
+1 for option 3 followed by 4. Practically speaking it would be a form of deprecation, with the validator issuing deprecation warnings (that ideally will say when the old format will become unsupported), right? If this works for this case it might be a general way to approach backward-incompatible changes on a case by case basis instead of waiting for 2.0. |
Hi all, I'm a bit late on this. I wanted to comment on the initial issue , but it is now locked, so I'll comment here, from a few points that were touched:
Finally, in the initial discussion, there was one big point that was about deprecation, coexisting Thanks for this, I think it will be of grat help! |
I have rebased this on top of #624, following conversations there. For a clean diff see oesteban/bids-specification@enh/dwi-combinations...mattcieslak:dwi_tsv |
@bids-standard/raw-mri-dwi @bids-standard/raw-mri I have recently received a notification for this (I believe) alternative BEP in development - https://arxiv.org/pdf/2103.14485.pdf which I think addresses at the very least some of the problems we are targeting here. WDYT about the preprint overall, @mattcieslak? should we just close this and ask the authors of the preprint to submit their proposal as a PR? |
@arokem, @francopestilli, @Lestropie, @mattcieslak, @satra, @edickie @dlevitas at a first glance the proposal seems good. It also appears to propose additions to the current spec and not necessarily steps towards what we were trying to do (from my point of view). I will read it in more detail, but what do the others think? |
Looks like it very thoroughly addresses the comment on the issue that precipitated this PR. It would be nice to start thinking about these things.
I agree with @francopestilli's observation that this might be somewhat orthogonal to what is done here. Many of the things that are proposed are also not crucial for many methods. Maybe this PR could be completed in parallel (or first?)? I think there might be ways in which incorporating their ideas could build upon what is proposed here. |
The preprint looks very ambitious and covers diffusion encoding schemes that are still new and fairly rare. The main goal of this PR is to have a definite mapping between gradient directions and LR/AP/IS that is independent of the image axis. This should be easy in most cases to get out of the dicoms and fits nicely into a simple tsv, without adding lots of other features |
Thanks! Let's get this done then and we'll see how the preprint fits
independently.
…On Tue, Apr 6, 2021, 15:04 Matt Cieslak ***@***.***> wrote:
The preprint looks very ambitious and covers diffusion encoding schemes
that are still new and fairly rare. The main goal of this PR is to have a
definite mapping between gradient directions and LR/AP/IS that is
independent of the image axis. This should be easy in most cases to get out
of the dicoms and fits nicely into a simple tsv, without adding lots of
other features
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#352 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAESDRUVZ72LBISH3WEMIC3THMBGTANCNFSM4JCUTX4A>
.
|
@oesteban @arokem @mattcieslak it looks like we agree. Interesting but orthogonal. A BIDS for Pros. |
Hi all, I'd planned to share this preprint explicitly rather than indirectly. Overcoming difficulties such as the subject of this PR is intended as a central aim of this BEP. Recording parametric variation throughout a diffusion experiment is a necessary evil, and while stopgap solutions resolve temporary issues, ever more intricate sequences promise ever more detailed parametric variation. We've tried to show in the manuscript how the many types of diffusion experiment cannot all be easily expressed in the same format of tabular file, and hence justify the modifiable structure proposed. We've structured things to suit option 2) here, extending and not superceding the bval / bvec system, which is used in a lot of pipelines we'd like to keep working. In general, the discussion here echoes a lot of conversations we've had internally, concerning paired tsv / json structures. Ultimately, this BEP would address this concern, and from the user's perspective, will mirror the suggestion proposed (of a more detailed tsv with more detailed parametric variation). This proposal isn't finished until it's seriously reviewed, so any analysis you offer is really appreciated. Please feel free to write to me with any suggestions. |
Thanks @JAgho for confirming that your BEP would extend/continue from this one. That clarifies the fate of this particular PR even further. Regarding the preprint, I think the BIDS steering committee and/or maintainers will reach out to you - particularly large BEPs typically require some discussion. Therefore, ensuring a flow of bi-directional feedback between BEP leads and the BIDS community is really important for them to succeed. |
@JAgho @oesteban if possible we would love to stay informed because of the work we have done before and plan to do later @arokem @mattcieslak @Lestropie @jchoude |
…ata.md Co-Authored-By: Oscar Esteban <[email protected]>
Okay, I have added one line about the "effective" vs "requested" gradients in (see 487032c). |
The gradient strength and orientation information corresponding to a DWI acquisition | ||
is REQUIRED, and therefore, MUST be stored in the structure. | ||
The gradient strength and orientation information stored in the structure MUST correspond | ||
to "*effective*" values (as in, the actual orientations and strengths applied by the scanner), | ||
rather than the "*requested*" values (as in, the original gradient table design that is | ||
configured into the scanner's MR protocol settings). | ||
|
||
The RECOMMENDED way to store gradient information is a `[*_]dwi.tsv` file corresponding | ||
to one or more DWI files. | ||
The format of this TSV file, and associated metadata, are described below. | ||
The gradient information MAY be stored using the `[*_]dwi.bval` and `[*_]dwi.bvec` pair | ||
of files. | ||
However, the `[*_]dwi.[bval|bvec]` files are DEPRECATED and will be | ||
phased out in a future release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a breaking change shouldn't this be under the 2.x consideration? if not perhaps the wording at present could be changed to support both the current required bval/bvec and add the tsv file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this PR more as a bugfix (and hence that takes precedence over the backward compatibility principle). Happy to see proposals on better wording though.
That said, with the current text both would be supported. TSV would take precedence over bval/bvec, and implicitly (but not explicitly), the tsv should be equivalent to bval/bvec. The only "breaking" change is lifting the mandate to have bvec/bval files (if you have the tsv)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satra did my reply address your concern? How would you address this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry too many things going on. my suggestion would be to continue to require bval and bvec till bids 2.0. regarding it being a bug. is it really a bug, or just that it forces a certain consideration that does not work in all situations. i.e. software have to be aware of the details of the spec. if the latter, that to me is not a bug but an alienation problem, which can be addressed by making the tsv required/recommended moving forward. it's possible i'm not completely understanding the bug part of the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW - how does this PR breaks backward compatibility? The deprecation is exactly to tolerate the utilization of the old feature while issuing a strong recommendation of the replacement (https://en.wikipedia.org/wiki/Deprecation) - precisely to avoid breaking changes.
I'd agree with removing the .bvec
/.bval
only the 2.0 release (which would indeed be the breaking change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a breaking change in the specification is one where existing tooling stops working as a result of the change. typically deletions of required elements in any schema are breaking changes. additions are also breaking changes if the new keys introduced are required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a breaking change in the specification is one where existing tooling stops working as a result of the change.
Yup, and since .bval/.bvec will still be allowed, a dataset that currently works will continue working on the same tool. The requirement is to have at least one of the two options (.tsv or .bval/.bvec) - so if the .tsv is not present, the specification remains exactly the same.
The new format is RECOMMENDED, and will only be mandatory when the deprecation finishes. Possibly BIDS 2.0.
The only effect of this PR is that now a BIDS dataset can be written with a much better, unambiguous format for the gradients. And that's not breaking compatibility because scanner coordinates could not be encoded (without conversion to voxels) before.
Anyhow, besides I personally think this is not breaking compatibility, there's also the fact that we indeed discussed how this deprecation would happen when Matt first submitted the PR. It would be unfortunate that we revisited those decisions (for the reasons above, and because that will set some precedent as to maybe preempt future contributions).
My impression is that although this proposal has important substance changes (I particularly like @jchoude's summary above), we seem to be focusing more on the formal aspect of the deprecation (which has largely been discussed already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i completely agree that this sub discussion has to do with principles of BIDS rather than the substance of this PR. however, if this PR is merged software that worked prior to this could stop working. for example, wouldn't this mean that after this change none of the scripts using the FSL tools will work on new datasets unless someone did an appropriate conversion?
most software doesn't check on bids version for a dataset given that bids has not released 2.0. i think that's the fundamental question in this thread. this PR just happens to be an instance of something that brings this up. i don't want to hold up this PR, but it seems that there are other instances where such an issue could come into play. as bids expands this could happen at a modality specific level such as this, or the common principles level (there were a few inheritance issues that were pushed off to bids 2.0).
thus this is a meta discussion for bids as a whole, not just this PR. but merging this PR, means we have accepted this route at least for one instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example, wouldn't this mean that after this change none of the scripts using the FSL tools will work on new datasets unless someone did an appropriate conversion?
For existing datasets, not at all - this PR does not mandate the update from bvecs/bvals to tsv.
For future datasets, you can generate the bvecs/bvals from the tsv file. Making this step more explicit will make it harder for the user, but will also ensure data is preserved as it was generated by the scanner (in the tsv file).
If someone is motivated and has the knowledge to implement a test within the BIDS validator to ensure that the tsv and bvec/bval are consistent, that would be really helpful for the DWI community.
(corresponding to the *N* volumes in the corresponding NIfTI file.) | ||
The first row contains the *x* elements, the second row contains the *y* elements and | ||
the third row contains the *z* elements of a unit vector in the direction of the applied | ||
diffusion gradient, where the *i*-th elements in each row correspond together to | ||
the *i*-th volume, with `[0,0,0]` for *non-diffusion-weighted* (also called *b*=0 or *low-b*) | ||
volumes. | ||
Following the FSL format for the `[*_]dwi.bvec` specification, the coordinate system of | ||
Following the FSL format for the `.bvec` specification, the coordinate system of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The directions in the bvec aren't always using the image axis coordinate system. @frankyeh recently updated DSI Studio to handle the case where the image axes aren't in the FSL "RPI" orientation but the bvecs are. FSL interprets bvecs differently depending on the orientation of the image axis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I didn't know that. I think this just reinforces the argument that this PR is necessary. How the (recently added) conversion example would change with this? Should we mention the image axis coordinate system so that what is said is fully accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion example will only work if the image is in LAS+ orientation. This will be true by default if dcm2niix was used, but is not guaranteed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is the actualFSL spec: https://fsl.fmrib.ox.ac.uk/fsl/fslwiki/FDT/FAQ#What_conventions_do_the_bvecs_use.3F (it should be included in that section)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional description here: https://community.mrtrix.org/t/bug-in-fsl-bvecs-handling/165?source_topic_id=1434
I follow up on @satra The PR seems to stem from the learning and implementation of new code (I assume). The new code might have better features than the older code (e.g., coordinate system). Yet, as it is new code, it is not embraced by the community yet, it is not known yet and it has not involved the community yet. So this is a good general discussion. How do we propose changes to the specification that would accommodate new code and general improvements when the code is not out yet, the code has the need been tested either by the BIDS contributor not by the community at large. Good food for thought I hope. |
Is it always possible to take a new-style gradient TSV and input images and generate a correct set of FSL-style bval/bvecs? If so, then making a small tool that does that might be the cleanest option. bval/bvec can remain REQUIRED which means that old tools that take the standard at its word and assume the files can be found, while users can start with an authoritative TSV file. We have DEPRECATED things in recent releases, but we have not previously deprecated REQUIRED data. Probably the closest is We may not be at voting stage yet, but I think I would probably be inclined to say that bval/bvec remain REQUIRED. |
If it's impracticable to validate consistency, we could say that, for tools that accept either TSV or bval/bvec, the TSV is to be considered authoritative. |
It wouldn't be hard to make a small tool to translate between bval/bvec files and TSVs. MRtrix3 already has a really good one that even handles oblique acquisitions. It also might not be totally correct to say that there is already a lot of software support for the bvec format. MRtrix (uses the strict FSL spec) and DIPY (uses image axes) handle them differently. DSI Studio has used handled them both ways at different times. DSI Studio, MRtrix and AFNI all have tools that permute the signs of the bvecs to figure out empirically how to interpret them |
@mattcieslak I understand the complexity in handling coordinate systems, please do not het ,e started :-) ... yet all of these tools you mention read in bvecs, correct? @effigies I would say the mapping is lossless this way bvecs -> TSV (but it requires more than just BVECS to know the coordinate system) and is lossy this way TSV -> BVECS (as it loses the coordinate system). Does this help? |
If I may chime in on this very important, long-due improvement:
Thanks for your work on this! |
Of course! Thanks for doing so.
I may have this wrong, but LPS+ determines the orientation of the voxel cartesian space, while the physical (world) coordinates system is right-handed and defined w.r.t. the scanner's bore, right?. I would agree though that the proposed header (R, A, S) is unfortunate because actually confuses the image's cartesian grid system with the world system. It would be better to just say X, Y, Z (as opposed to I, J, K that is proposed for voxel coordinates).
Yes, this is probably one of the weakest points of this PR. Please send suggestions to clarify this. |
Sorry I was probably mixing terminology/conventions and not as familiar with the BIDS conventions. So to clarify my point -- independent of the matrix that defines the locations of voxels in space (which could be described in shorthand as RAS+, ALS+, RPI+, etc. where the letters indicate the rough direction in which higher-indexed voxels go), there is the coordinate system defining what the coordinate (x, y, z) means and the convention in NIFTI is where positive numbers mean R, A, and S respectively, which I would also find useful to describe as RAS+ (as opposed to DICOM's LPS+). I actually like that the columns would use R, A, and S as the convention is then explicit in the file. The alternative would be to embed the transformation from gradient directions into world-space (what NRRD would call the "measurement frame") but that would diverge with the "image space" option (with I, J, K columns) and so I think the current approach is simple enough and easy to understand.
I'll try to put some text together. Thanks! |
@SyamGadde any chance you find a slot to put together the edits? |
Can you tell me where to target the changes? I was having trouble figuring out how to put together a pull request for this issue and I sent one to the original branch: Happy to resubmit, just need to know where... |
Sorry I missed that. I'll go through it tomorrow |
Coming here for the first time due to bids-standard/bids-bep016#26; apologies for missing the discussion to this point after having initially lodged #243, GitHub notifications weren't making it to my inbox.
|
Just to quickly clarify a statement from @Lestropie - since he saw fit to tag me in...
Yes and no. First off, apologies if I'm being a bit pedantic or telling everyone what they already know... In the official DICOM specs, and for Philips and Siemens, the gradient vectors are indeed in XYZ, which corresponds to the axes of the scanner hardware in the patient-centered coordinate system (PCS) (official docs here)¹. This only corresponds to the actual hardware coordinate system if patients are positioned head-first supine, otherwise it will be some permutation of these axes (this depends on what the radiographer specifies in the positioning field when registering the patient). But in general, this actually simplifies things, since we can always rely on the assumption that (in DICOM) X is R→L, Y is A→P, and Z is I→S (i.e. LPS+, as others have said). In practice, I expect this is what most people mean when talking about the XYZ coordinate system, since we overwhelmingly deal with HFS positioning (certainly true in my case) - but in my opinion it's more accurately described as RAS+ (though I acknowledge the ambiguity that remains concerning whether we mean scanner or image axes - I mean scanner axes here). Otherwise, I would echo what others have said elsewhere:
One final point which I don't think has been raised so far is how to handle acquisitions where the magnitude of the gradient vector is modulated to achieve lower b-values than nominally requested - this is to allow multi-shell acquisitions on systems that would otherwise not easily support this (what we call b-value scaling in MRtrix). I may have missed it, but has there been any discussion as to how this should be handled in the spec - if at all? ¹As a side note: before the official DICOM standard, GE would store their gradients in the PRS frame. |
"BigDelta": 0.005, | ||
"SmallDelta": 0.0005 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Given that these appear in a general sidecar metadata space rather than a DWI-specific context, the names "
BigDelta
" and "SmallDelta
" may not be specific enough. -
If revisiting, it would be worth considering whether it is worthwhile adding these, or instead deferring more advanced diffusion encoding information entirely to the aDWI extension.
Adds a dwi tsv option with gradient directions in physical coordinates instead of image axes. Addresses #349.